As I continue testing with this, I am doubting that my simple solution to the issue in #6487 will be sufficient now that 1.10 is out the door and this PR would go in 1.11. @htuch @gsagula - could use advice here.

Here is where I am having issues...

I'm building my gRPC service (in go) to be 1.11 compatible with this field and so I can call (in go) func GetPartialBody() bool no matter what version of envoy the client is using. One issue I am seeing as I am testing more with this feature is that I cannot rely on the partial_body bool field when envoy 1.10 is a client as the field will always be false (field not sent - defaults false). This seems to pose a problem with proto3 as there is no way to detect a non-existent scalar field like proto2. Seems in proto3 there are two ways around this (none of which is overly appealing):

• Use an enum allowing for a tri-state value where the first (zero; default value) is NOTSET and then a FALSE and TRUE value
• Use oneof (which allows testing for partial_body_available() == nil indicating envoy 1.10 clients)

oneof partial_body_available {
  bool partial_body = 12;
}

This is generally an issue with the new body field as well. I can call func GetBody() string in go and it will return the empty string for envoy <= 1.10 as a client and there is no way to detect if this is just "no body" or if it is an envoy too old for this feature (and thus should use another workaround I have).

Im now realizing I should have been testing this more as #5824 was progressing. It may have been better to do something similar to the tap filter where the body is not a scalar type and thus not subject to this problem (i.e., it can be nil). Perhaps better is to swap out body for a HttpBody type:

message AttributeContext {
  ...

  message HttpRequest {
    ...

    string body = 11 [deprecated = true];

    // Replace `body` with a non-scalar type to detect availability of the field
    message HttpBody {
      string data = 1; // Possibly: oneof data { string as_string = 1; }
      bool partial = 2;
    }
    HttpBody http_body = 12;
}

This would allow (in go) GetHttpBody() to return nil in the case of it not being set and allow for detection of the body not being available vs an empty body. Thoughts?

As I continue testing with this, I am doubting that my simple solution to the issue in #6487 will be sufficient now that 1.10 is out the door and this PR would go in 1.11. @htuch @gsagula - could use advice here.

Here is where I am having issues...

I'm building my gRPC service (in go) to be 1.11 compatible with this field and so I can call (in go) func GetPartialBody() bool no matter what version of envoy the client is using. One issue I am seeing as I am testing more with this feature is that I cannot rely on the partial_body bool field when envoy 1.10 is a client as the field will always be false (field not sent - defaults false). This seems to pose a problem with proto3 as there is no way to detect a non-existent scalar field like proto2. Seems in proto3 there are two ways around this (none of which is overly appealing):

• Use an enum allowing for a tri-state value where the first (zero; default value) is NOTSET and then a FALSE and TRUE value
• Use oneof (which allows testing for partial_body_available() == nil indicating envoy 1.10 clients)

oneof partial_body_available {
  bool partial_body = 12;
}

This is generally an issue with the new body field as well. I can call func GetBody() string in go and it will return the empty string for envoy <= 1.10 as a client and there is no way to detect if this is just "no body" or if it is an envoy too old for this feature (and thus should use another workaround I have).

Im now realizing I should have been testing this more as #5824 was progressing. It may have been better to do something similar to the tap filter where the body is not a scalar type and thus not subject to this problem (i.e., it can be nil). Perhaps better is to swap out body for a HttpBody type:

message AttributeContext {
  ...

  message HttpRequest {
    ...

    string body = 11 [deprecated = true];

    // Replace `body` with a non-scalar type to detect availability of the field
    message HttpBody {
      string data = 1; // Possibly: oneof data { string as_string = 1; }
      bool partial = 2;
    }
    HttpBody http_body = 12;
}

This would allow (in go) GetHttpBody() to return nil in the case of it not being set and allow for detection of the body not being available vs an empty body. Thoughts?

@brectanus-sigsci We could add it as header metadata, something like x-envoy-auth-partial: 100. Where value indicates the size of the partial message. If this header is added in check request, all clients (HTTP and gRPC) will have the same feature. WDYT?

@brectanus-sigsci We could add it as header metadata, something like x-envoy-auth-partial: 100. Where value indicates the size of the partial message. If this header is added in check request, all clients (HTTP and gRPC) will have the same feature. WDYT?

@gsagula Yes. Just came to that conclusion responding to your other comment. I am not sure a length adds anything here as you can get the length from body. Maybe just x-envoy-auth-partial-body: 0|1? And then I can detect non-existance via missing header metadata.

Sure. I thought about passing the size because it's useful info I guess, but either works. Up to you :)

Sure. I thought about passing the size because it's useful info I guess, but either works. Up to you :)

Would the size in the metadata ever be different than the string length of body in gRPC or the content-length header in the HTTP case? Otherwise it seems like redundant info. Originally I had debated this instead of the bool, but could not come up with a scenario where it made sense. It seems the full un-truncated content length might make sense to see how much you are missing, but I don't think that would always be available, so I settled on just bool. I'll stick with bool unless someone can think of a scenario where the length is not redundant. Will update the PR shortly. Thanks!

Sure, thanks for tackling it :)

@gsagula I updated to using a metadata header as we discussed to get around some 1.10 compatibility issues I found after more testing.

@htuch The concept changed, so you may want to re-review :)

Awesome! Just one small comment about preventing x-envoy-auth-partial-body to be added outside the filter.

OK. Fix for this pushed with updated tests.

🔨 rebuilding ci/circleci: release (failed build)

/retest

🔨 rebuilding ci/circleci: coverage (failed build)
🔨 rebuilding ci/circleci: docs (failed build)

@brectanus-sigsci sorry needs a master merge.

/wait

@mattklein123 conflict resolved. Hopefully checks pass the first time :)